Skip to content

Build the coverage line table in UTF-16 code units - #38710

Closed
robobun wants to merge 3 commits into
farm/612517fd/16bit-sourcesfrom
farm/612517fd/coverage-utf16
Closed

robobun wants to merge 3 commits into
farm/612517fd/16bit-sourcesfrom
farm/612517fd/coverage-utf16

Conversation

@robobun

@robobun robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator

Split out of #33866. Based on #38708 for ordering only.

Problem

  • bun test --coverage reports the wrong lines for a file whose text has a non-ASCII character. A licence banner (/*! © ... */) is enough: each line after it shifts, and functions that never run vanish from the lcov record.
  • JSC reports block ranges in code units of the source string that it holds. ByteRangeMapping (src/sourcemap_jsc/CodeCoverage.rs) looked them up in a table of byte offsets into the UTF-8 form of that string. The two agree only for ASCII text.

Fix

  • The mapping stores the start of each line in code units. ASCII text uses the byte offsets as they are. Other text has each line measured again with element_length_utf8_into_utf16.
  • Correct for both widths of JSC string: the count of UTF-16 code units of the UTF-8 form equals JSC's offsets.
  • Verified: "a non-ASCII preserved comment does not shift coverage lines" in test/cli/test/coverage.test.ts. It fails on main.

Background

  • A JSC string is 8-bit (Latin-1) or 16-bit (UTF-16). Its offsets are indexes into the string. The UTF-8 form is longer as soon as one character is above 0x7F.
  • An lcov DA:<line>,<count> record is what --coverage-reporter=lcov writes for each line.

Downsides

  • Only with --coverage, per loaded file: one more scan of the text (is_all_ascii) and one copy of 4 bytes per line. Non-ASCII text gets a second scan.
  • None found for memory. The mapping keeps 4 bytes per line, where it kept the whole line offset table before.
Notes

What the test does

  • It runs the same module twice, once with an ASCII banner and once with a ©🐰 banner of the same length in code units (an astral character makes code units differ from both bytes and code points). It requires identical lcov records, after a check that the ASCII record has the two functions that never run on lines 7 and 10.
  • On main the records differ: DA:7,0 and DA:10,0 become DA:4,0, DA:5,0 and DA:6,0.
  • The helper takes variants, so the regex and tagged template variant (which needs Preserve non-ASCII in regex .source and tagged template .raw #33866) is a second test there.

Names

  • The byte_offset locals in the report loops and the name ByteRangeMapping follow JSC's own "byte range" term for these offsets. They stay as they are. The unit is documented on the field they are compared with.

Rebase of 2026-10-02


[review] gate passed · iteration 18 · 3 files touched

fails on main (without fix)
ASAN without fix: 1 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/cli/test/coverage.test.ts
bun test v1.4.1 (861e9ae04)

test/cli/test/coverage.test.ts:
bun test v1.4.1 (861e9ae04)

demo.test.ts:
--------------|---------|---------|-------------------
File          | % Funcs | % Lines | Uncovered Line #s
--------------|---------|---------|-------------------
All files     |    0.00 |   66.67 |
 demo.test.ts |    0.00 |   66.67 | 
--------------|---------|---------|-------------------

 0 pass
 0 fail
Ran 0 tests across 1 file. [215.00ms]
(pass) coverage crash [323.38ms]
bun test v1.4.1 (861e9ae04)

demo2.ts:

 0 pass
 0 fail
Ran 0 tests across 1 file. [228.00ms]
(pass) lcov coverage reporter [281.45ms]
(pass) coverage excludes node_modules directory [269.85ms]
(pass) coveragePathIgnorePatterns - single pattern string [374.41ms]
(pass) coveragePathIgnorePatterns - partial coverage without nan [357.18ms]
(pass) coveragePathIgnorePatterns - array of patterns [402.83ms]
(pass) coveragePathIgnorePatterns - glob patterns [384.15ms]
(pass) coveragePathIgnorePatterns - lcov reporter [296.44ms]
(pass) cov
... (truncated)

release without fix: 1 FAILED
bun test v1.4.1-canary.1 (861e9ae04)

test/cli/test/coverage.test.ts:
bun test v1.4.1-canary.1 (861e9ae04)

demo.test.ts:

 0 pass
 0 fail
Ran 0 tests across 1 file. [12.00ms]
(pass) coverage crash [32.44ms]
bun test v1.4.1-canary.1 (861e9ae04)

demo2.ts:

 0 pass
 0 fail
Ran 0 tests across 1 file. [5.00ms]
(pass) lcov coverage reporter [51.65ms]
(pass) coverage excludes node_modules directory [49.43ms]
(pass) coveragePathIgnorePatterns - single pattern string [49.06ms]
(pass) coveragePathIgnorePatterns - partial coverage without nan [40.96ms]
(pass) coveragePathIgnorePatterns - array of patterns [40.10ms]
(pass) coveragePathIgnorePatterns - glob patterns [16.61ms]
(pass) coveragePathIgnorePatterns - lcov reporter [23.55ms]
(pass) coveragePathIgnorePatterns - invalid config type [12.29ms]
(pass) coveragePathIgnorePatterns - invalid array item [2.65ms]
(pass) coveragePathIgnorePatterns - empty array [52.16ms]
(pass) coveragePathIgnorePatterns - ignore all files [18.12ms]
(pass) --parallel merges line coverage across workers [129.41ms]
721 |     legalComment: coverageDemo(repeatUnits("©🐰", coverageBannerUnits), asciiText),
722 |   });
723 |   // `uncovered` and `a
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/cli/test/coverage.test.ts
bun test v1.4.1 (861e9ae04)

test/cli/test/coverage.test.ts:
bun test v1.4.1 (861e9ae04)

demo.test.ts:
--------------|---------|---------|-------------------
File          | % Funcs | % Lines | Uncovered Line #s
--------------|---------|---------|-------------------
All files     |    0.00 |   66.67 |
 demo.test.ts |    0.00 |   66.67 | 
--------------|---------|---------|-------------------

 0 pass
 0 fail
Ran 0 tests across 1 file. [212.00ms]
(pass) coverage crash [311.76ms]
bun test v1.4.1 (861e9ae04)

demo2.ts:

 0 pass
 0 fail
Ran 0 tests across 1 file. [333.00ms]
(pass) lcov coverage reporter [396.28ms]
(pass) coverage excludes node_modules directory [357.51ms]
(pass) coveragePathIgnorePatterns - single pattern string [291.31ms]
(pass) coveragePathIgnorePatterns - partial coverage without nan [310.18ms]
(pass) coveragePathIgnorePatterns - array of patterns [296.72ms]
(pass) coveragePathIgnorePatterns - glob patterns [310.62ms]
(pass) coveragePathIgnorePatterns - lcov reporter [302.01ms]
(pass) cov
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     fabdf83a01
  features     baseline

23 deps, 129 codegen, 1172 objects in 662ms

ninja: Entering directory `/workspace/bun/build/release'
[1/1244] install /workspace/bun
bun install v1.4.1-canary.1 (861e9ae04)

Checked 26 installs across 63 packages (no changes) [3.00ms]
[2/1244] gen ErrorCode+*.h
[3/1244] gen bindgenv2
[4/1244] install /workspace/bun/packages/bun-error
bun install v1.4.1-canary.1 (861e9ae04)

Checked 1 install across 2 packages (no changes) [1.00ms]
[5/1244] gen .bind.ts → GeneratedBindings.cpp
[6/1244] fetch libjpeg-turbo
[libjpeg-turbo] up to date
[7/1217] fetch zlib
[zlib] up to date
[8/1217] install /workspace/bun/src/node-fallbacks
bun install v1.4.1-canary.1 (861e9ae04)

Checked 111 installs across 104 packages (no changes) [12.00ms]
[9/1217] fetch tinycc
[tinycc] up to date
[10/1216] gen bake.{client,server,error}.js
-> bake.client.js, bake.server.js, bake.error.js
[11/1216] gen node-fallbacks/react-refresh.js
Bundled 1 module in 6ms

  react-refresh.js  4.81 
... (truncated)
diff hotspot
src/runtime/cli/test_command.rs   |  4 +-
 src/sourcemap_jsc/CodeCoverage.rs | 41 ++++++++++++++++---
 test/cli/test/coverage.test.ts    | 84 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+), 8 deletions(-)

gate history · 2 passed · 0 rejected · iteration 18

evidence per changed file
file                               reads  edits  tests
src/runtime/cli/test_command.rs        0      0      0
src/sourcemap_jsc/CodeCoverage.rs      3      3      0
test/cli/test/coverage.test.ts         4      4      0

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. The encoding math checks out and the test is well-constructed, but since this is a runtime logic change with a struct field replacement and explicit drop_elements() handling, a human sign-off would be worthwhile.

What was reviewed:

  • Verified the code-unit re-measurement is correct for both Latin-1 and UTF-16 JSC strings (© = 1 unit / 2 UTF-8 bytes; 🐰 = 2 units / 4 bytes; element_length_utf8_into_utf16 via simdutf agrees).
  • Checked find_index is a plain binary search over sorted u32 offsets, so feeding it code-unit offsets instead of byte offsets is sound.
  • Confirmed no other consumers of the removed line_offset_table field; Box::from(byte_starts) copies before drop_elements(), and the borrow ends before the &mut call.
  • The comment at test_command.rs:1598 ("owns a MultiArrayList") is now stale but the code it justifies is unchanged and still correct.
Extended reasoning...

Overview

This PR fixes coverage line misattribution for source files containing non-ASCII characters. ByteRangeMapping in src/sourcemap_jsc/CodeCoverage.rs previously stored a LineOffsetTable (byte offsets of line starts) and looked up JSC's code-unit block offsets against it — the two units only agree for pure ASCII. The struct now stores line_starts: Box<[u32]> in UTF-16 code units, computed by re-measuring each line with strings::element_length_utf8_into_utf16 when the source is not all-ASCII. The full LineOffsetTable (including per-line columns_for_non_ascii boxes) is now freed immediately via drop_elements() rather than living in the thread-local map. A regression test in test/cli/test/coverage.test.ts runs identical modules with ASCII vs. ©🐰 banners of equal code-unit length and requires identical lcov DA: records.

Security risks

None. This is pure offset arithmetic on already-decoded module source text; no new inputs, no parsing of untrusted data, no allocation sizing driven by external values beyond what already existed.

Level of scrutiny

Medium. This is a real algorithmic change to runtime code, not a mechanical tweak. The reasoning (JSC 8-bit Latin-1 and 16-bit UTF-16 both round-trip through UTF-8 to the same code-unit count JSC reports) is correct but subtle enough that a maintainer should confirm the approach — particularly since the column_position computed in the sourcemapped branch is now in code units rather than the previous mixed-unit subtraction, which is more correct but interacts with the internal-sourcemap column semantics that PR 4 of the stack (#33866) addresses.

Other factors

  • The struct field change is fully contained: grep confirms no other reader of .line_offset_table on ByteRangeMapping; the only external references to the type (test_command.rs, C++ FFI in ZigSourceProvider.cpp / InternalModuleRegistry.cpp) go through unchanged accessors.
  • Memory handling is sound: Box::from(&[u32]) deep-copies before drop_elements() runs; drop_elements() sets len = 0 so the subsequent MultiArrayList Drop only frees the slab.
  • The test follows harness conventions (tempDir, bunEnv, concurrent spawns with drained stderr, content assertion before exit-code assertion) and asserts specific DA:7,0 / DA:10,0 lines on the ASCII baseline before the equality check, so it cannot pass vacuously.
  • Minor: the loop variable byte_offset and line_start_byte_offset in generate_report_from_blocks are now misnomers (they hold code units), and test_command.rs:1598's comment is stale — neither affects correctness.

Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/sourcemap_jsc/CodeCoverage.rs Resolved
Comment thread src/sourcemap_jsc/CodeCoverage.rs Resolved
Comment thread src/sourcemap_jsc/CodeCoverage.rs Resolved
@robobun
robobun force-pushed the farm/612517fd/coverage-utf16 branch from f856ca8 to 785825f Compare August 14, 2026 22:22
Comment thread src/runtime/cli/test_command.rs Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. The encoding logic checks out (UTF-16 code-unit count of the UTF-8 re-encoding equals JSC's offset for both 8-bit Latin-1 and 16-bit strings), and the test exercises both BMP and astral characters. Because it's part of a 4-PR stack and the correctness argument hinges on JSC string-representation invariants, a human look would still be worthwhile.

What was reviewed:

  • compute() conversion: verified element_length_utf8_into_utf16 yields the right count for Latin-1→UTF-8 (© = 2 bytes → 1 unit) and surrogate pairs (🐰 = 4 bytes → 2 units), matching JSC's index unit in both storage modes.
  • drop_elements() ordering: byte_starts is copied into the Box<[u32]> before the mutable borrow; the slab itself is freed by MultiArrayList::Drop at scope exit, so no leak or UAF.
  • No other consumers of the removed line_offset_table field; find_index is agnostic to the offset unit.
  • The comment-cop bot flags look like false positives — the flagged comments are 2-3 line field/unit documentation, not workaround justifications.
Extended reasoning...

Overview

This PR changes ByteRangeMapping in src/sourcemap_jsc/CodeCoverage.rs to store line-start offsets in UTF-16 code units instead of UTF-8 bytes, fixing coverage line misattribution when source text contains non-ASCII characters. The struct's line_offset_table: MultiArrayList<...> field becomes line_starts: Box<[u32]>; compute() now branches on is_all_ascii and re-measures each line via element_length_utf8_into_utf16 when needed. A comment in test_command.rs is reworded to match the new field, and a new differential test in coverage.test.ts compares lcov records for an ASCII vs. ©🐰 banner of equal code-unit length.

Security risks

None. This is offline coverage-report post-processing over source text the runtime already loaded; no new parsing of untrusted input, no auth/crypto/network paths.

Level of scrutiny

Medium-high. The change is small in line count but rests on a non-obvious encoding invariant: that for both JSC string kinds (8-bit Latin-1 and 16-bit UTF-16), the UTF-16 code-unit length of the UTF-8 re-encoding equals the index unit JSC reports. I verified this holds (Latin-1 chars are all BMP → 1 unit each; surrogate pairs round-trip to 2 units), and the PR description states it clearly, but it is exactly the kind of claim a maintainer familiar with the JSC source-provider path should confirm. The column_position calculation in the sourcemap branch now subtracts two code-unit values instead of the previous mixed byte/code-unit subtraction — strictly more correct, but worth a maintainer glance since sourcemap column semantics are involved. The drop_elements() addition changes when per-row column boxes are freed; borrow ordering is correct (the &[u32] is consumed before the &mut call) and the slab is still freed by the list's own Drop.

Other factors

  • Second of a 4-PR stack split from #33866; the description notes the helper is shaped for a follow-up variant test, so a human should confirm the split boundaries are what they want.
  • The new test is well-constructed: asserts specific DA:7,0 / DA:10,0 lines on the ASCII baseline before comparing, uses an astral character so code-unit ≠ codepoint ≠ byte, and drains stderr/exit concurrently.
  • comment-cop left five automated "paragraph-long comment" flags; on inspection each is a short (2-3 line) unit-documentation comment rather than a workaround justification, so I read them as bot noise, but they are technically outstanding on the thread.

@robobun

robobun commented Aug 15, 2026 •

Copy link
Copy Markdown
Collaborator Author

Rebased onto #38708's head a865431 (main 4b02e10) as aa48a4a on 2026-10-02. One commit.

CI for aa48a4a, #122756: 181 of 182 jobs green. The one red job is the x64-asan test lane, where test/js/bun/spawn/spawn.test.ts (an idle stdout reader of an unref'd child) failed. Nothing in this diff runs outside bun test --coverage reporting. Reported for triage.

@alii alii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the code-unit invariant for both string shapes JSC can hold here: an 8-bit string with characters above 0x7F (to_utf8 re-encodes each byte to two, element_length_utf8_into_utf16 counts one unit each) and a 16-bit string (astral pairs four bytes / two units, lone surrogates U+FFFD / one unit); both are 1:1 with JSC's offsets. Every other offset use in CodeCoverage.rs now compares against the unit table, including the sourcemap-branch column, which matches what Bun's own maps emit. drop_elements is a real method, frees the per-row column boxes MultiArrayList's Drop leaves behind (a small pre-existing leak), and the later Drop is slab-only. The test fails on 1.3.14 exactly as described (DA:7/DA:10 vs DA:4/5/6, FNH identical). Fine to land from my side.

Nits: the body says the diff is two files but the second commit also touches src/runtime/cli/test_command.rs (a comment); is_all_ascii re-scans text LineOffsetTable::generate already classified (its first-non-ASCII column would do), once per file under --coverage so not worth blocking on; repeatUnits silently needs units % piece.length == 0, both call sites satisfy it, a one-line note would guard the follow-up variants.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks. Body now mentions the test_command.rs comment. repeatUnits asserts units % piece.length === 0 in #33866, which is where the follow-up variants are added (the helper line is touched there anyway, so the check and its users land together rather than re-spinning this one). Leaving is_all_ascii as is per your note.

@robobun
robobun force-pushed the farm/612517fd/16bit-sources branch from 6f9b751 to 6f9b53b Compare August 25, 2026 21:58
@robobun
robobun force-pushed the farm/612517fd/coverage-utf16 branch from 785825f to fabdf83 Compare August 25, 2026 21:58

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — focused fix that rebuilds the coverage line table in UTF-16 code units so JSC's block offsets map to the right lines when the source contains non-ASCII text.

What was reviewed:

  • Verified the code-unit re-measurement is correct for both JSC string kinds: 8-bit Latin-1 (each char = 1 UTF-16 unit) and 16-bit UTF-16 — element_length_utf8_into_utf16 on the UTF-8 re-encoding recovers JSC's offset unit in both cases.
  • Confirmed MultiArrayList::Drop is slab-only, so the explicit drop_elements() correctly frees each row's columns_for_non_ascii box with no double-free (it zeroes len); the old code held the full table for the mapping's lifetime and leaked those boxes on drop.
  • Checked all four find_index call sites now compare like units (code-unit offsets vs. code-unit line starts); no other consumers of the removed line_offset_table field exist.
  • Test design: twin lcov comparison with a ©🐰 banner (30 code units either way; astral char distinguishes code units from both bytes and code points), non-vacuous baseline check on lines 7/10, using dir kept alive by the awaited Promise.all. Fails on main, passes on branch per the gate output.
Extended reasoning...

Overview

The PR fixes bun test --coverage line misattribution when module source text contains non-ASCII characters. JSC reports BasicBlockRange offsets in code units of the string it holds (Latin-1 bytes for 8-bit strings, UTF-16 units for 16-bit strings), but ByteRangeMapping::compute was building its line-start table from UTF-8 byte offsets of the re-encoded source. The two agree only for pure ASCII. The fix stores line starts in UTF-16 code units — using byte offsets directly when is_all_ascii holds, otherwise re-measuring each line via element_length_utf8_into_utf16. The struct field changes from a full line_offset_table::List to a plain Box<[u32]>; the intermediate MultiArrayList is drained with drop_elements() so its per-row columns_for_non_ascii boxes don't leak. test_command.rs gets a comment-only update to stop referring to the removed field.

Security risks

None. This is coverage-report line-number arithmetic in the CLI test runner; no untrusted input reaches new allocation sizes or indexing beyond what the existing code already did, and the only new arithmetic is a running u32 code-unit sum bounded by the source length.

Level of scrutiny

Medium. Encoding/offset math is subtle, but the change is small (~40 net lines of Rust), confined to ByteRangeMapping::compute plus a field-type swap, and the consumer loops in generate_report_from_blocks are unchanged — they already treated line_starts as an opaque monotone [u32] for binary search. I traced both JSC string encodings through the new path and they both reduce to the same UTF-16 code-unit count that JSC's offsets index. I also confirmed via multi_array_list.rs that Drop is slab-only and drop_elements() sets len = 0, so the explicit drain is correct and the subsequent implicit Drop only frees the slab.

Other factors

  • The new test is well-constructed: it compares full lcov records between an ASCII twin and a ©🐰 variant with identical per-line code-unit lengths, asserts the ASCII baseline reports the expected uncovered lines (7 and 10) so the equality check isn't vacuous, and uses an astral character so counting bytes or code points would fail. The PR body shows it failing on main and passing on the branch under both debug+ASAN and release.
  • The comment-cop bot warnings on the PR are a length heuristic; the flagged comments document the unit of line_starts and why drop_elements is needed — both are non-obvious and worth keeping.
  • Second of a four-PR stack; the description states nothing here depends on #38708 and the diff is self-contained.

…ix-up and the REPL

A module whose text contains any non-ASCII character (a preserved comment
is enough) is held by JSC as a 16-bit string. Three readers assumed 8-bit:

- ZigException.cpp only rendered the source preview for 8-bit sources,
  reading them through span8(); it now indexes the string, which works
  for both widths, so such modules get a preview again.
- adjustPositionBackwards in ErrorStackFrame.cpp gave up on 16-bit
  sources and reset the frame to 1:1 whenever moving a constructor
  call's position back to its new keyword crossed a line; the offsets
  it works with are code units in either width, so the 8-bit-only check
  is removed.
- Bun__REPL__evaluate decoded the program with WTF::String::fromUTF8,
  which returns a null string for ill-formed input; a preserved comment
  passes through the transpiler verbatim, so a stray byte in one made
  the whole input evaluate to nothing. It now uses the same decoder as
  the module loader, which substitutes U+FFFD.
@robobun
robobun force-pushed the farm/612517fd/16bit-sources branch from 6f9b53b to 6b80bea Compare October 1, 2026 18:01
…text

The excerpt scan in populateStackFramePosition starts at the error's
position, which is the length of the source when the expression it belongs
to ends the text (an identifier that is not defined, for example). The scan
then read one code unit past the source. Start it at the last code unit,
and skip the excerpt for an empty source. adjustPositionBackwards gets the
same bound for the same position.

8-bit sources had this read before. 16-bit sources reach it now that they
get an excerpt.
JSC reports the offsets of executed and unexecuted blocks in code units
of the source string it holds, but ByteRangeMapping looked them up in a
table of byte offsets built from the UTF-8 form of that string, so line
attribution drifted after any non-ASCII text in the module. Today a
non-ASCII preserved comment is enough: every line after it is
misattributed and functions that never ran are reported as covered.

The table now stores each line start in code units (the byte offsets
are kept as-is while the text is ASCII), and the per-row column tables
the line offset table allocates, which were never used here, are freed
instead of being kept for the life of the mapping.
@robobun
robobun force-pushed the farm/612517fd/coverage-utf16 branch from fabdf83 to aa48a4a Compare October 2, 2026 06:55

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟣 src/sourcemap_jsc/CodeCoverage.rs — Users running bun test --coverage on a module whose JSC text does not end in a newline get no line coverage for its final line, so an unexecuted last-line function never appears in the lcov record. In the loops at src/sourcemap_jsc/CodeCoverage.rs:762 and :808 every offset on the final line is fed to LineOffsetTable::find_index, which at src/sourcemap/LineOffsetTable.rs:134-149 only returns Some when an i + 1 entry exists or the offset equals the line start, and the equal case is then skipped by the >= guard at :771. Fix: treat offsets at or past the last entry of line_starts as belonging to the last line (fall back to the last index instead of None) in all four lookup loops, and add a fixture whose module lacks a trailing newline.

    Why this was flagged

    Trigger: a coverage-instrumented module whose text as JSC holds it has content on the last line with no trailing newline. Entry point: ByteRangeMapping::generate_report_from_blocks at src/sourcemap_jsc/CodeCoverage.rs:708, called for every covered file. For an offset on the last line, LineOffsetTable::find_index (src/sourcemap/LineOffsetTable.rs:121-150) finds byte_offset < loc_start but i + 1 < len is false, so it advances past the end and returns None; the else { continue; } at CodeCoverage.rs:767 drops the offset. The only offset that returns Some is the exact line start, which the >= guard at CodeCoverage.rs:771 then skips. Result: executable_lines and line_hits never see the last line, so lcov omits its DA: record and the function-level !did_fn_execute fill at :830-837 cannot mark it either. The base used the same find_index on the byte table, but this PR rewrote the table these loops consume without fixing the lookup that still drops the final line.

    Verification: Pre-existing. Trigger: the text JSC holds for a covered file has content on its last line and no trailing newline. src/sourcemap/LineOffsetTable.rs:121-150 find_index: for an offset strictly after the last entry's start, i + 1 < len is false, so it returns None; the only Some is the exact line-start hit, which src/sourcemap_jsc/CodeCoverage.rs:770-773 skips.

@robobun

robobun commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

On the review's note about a last line with no newline after it: I could not reproduce it. The text that JSC holds for a transpiled module ends with a newline, because the printer writes one after the last statement. Probe: a demo.ts whose last line is export function last() { return 2; } with no final newline gives the same lcov record as the same file with one (DA:4 is present in both). The lookup in LineOffsetTable::find_index is also not changed by this PR.

@robobun

robobun commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: #45677 fixes this on main.

@robobun robobun closed this Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants